On-demand loading of dynamic paths and local tasks #1

We need to re-purpose this issue and start with the basic implementation. Functionality is similar to contextual links in D7 core, but we want more. ;)

smk-ka already did some awesome work on this, so we just need to get that reviewed and in shape. Will post a patch soon.

You need to run update.php after applying this patch.

1) The structure of $expand_map looks awkward, and may very well be the cause for duplicated menu links for Field UI for content types.

2) Looks like we could skip MENU_DEFAULT_LOCAL_TASK with this, but OTOH, I originally envisioned that we want to stack dynamic links below the respective "List" task, so other tasks on the same level are not buried within a potentially large list of dynamic links.

3) Plenty of @todos.

#631550: Stale + improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB eliminated the need to cater for local tasks and actions. We've got almost 100% clean code now, yay! :)

So this patch needs to be revamped to only merge in router items with dynamic path arguments.

Started to streamline this code.

Text formats are currently not properly handled by (the wrongly named) function admin_menu_local_tasks(), because they are no local tasks.

I'm going to try to investigate whether we cannot totally streamline this code, since all dynamic router paths are properly stored as menu links now. This basically means that the following should get us a full-blown sub-tree for a certain dynamic path:

$path = 'admin/structure/types/manage/%';
$pids = db_select('menu_links', 'ml')
  ->fields('ml', array('p1', 'p2', 'p3', 'p4', 'p5', 'p6', 'p7', 'p8'))
  ->condition('router_path', $path)
  ->execute()->fetchAssoc();
$pids = array_filter($pids);

$query = db_select('menu_links', 'ml');
$query->join('menu_router', 'm', 'ml.router_path = m.path');
$query
  ->fields('ml')
  ->fields('m', array_diff(drupal_schema_fields_sql('menu_router'), drupal_schema_fields_sql('menu_links')));
foreach ($pids as $column => $pid) {
  $query->condition($column, $pid);
}
$result = $query->execute()->fetchAll(PDO::FETCH_ASSOC);

dsm($result);

Edited above code snippet: works :)

Actually, the $menu_name belongs to the things the still need to be removed. This dynamic tree expansion relies on router paths in the first place. In D7, also dynamic router paths are stored as menu links, so we just leverage that to retrieve the dynamic menu link tree, regardless of its menu_name.

Work in progress patch for the D7 approach attached... however, endless recursion in admin_menu_merge_tree(), since some expanded paths are not local tasks, but regular menu items, so they do not have a $parent_path, but the current code requires all registered dynamic paths to be keyed by their parent path.

Resolved the recursion error, removed obsolete helper functions, added plenty of docs.

Quick note: If we want to backport this to D6, then we need to backport #631550: Stale + improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB -- pwolanin, the menu system maintainer already agreed that it would be possible to do that.

Cleaned up field_ui implementation.

This patch requires #742318: Fields are editable regardless of whether an bundle instance exists, missing menu titles, etc. for Drupal core to make any sense.

With this patch, the only remaining problem is that

admin/structure/taxonomy/2/fields/field_image

gets also expanded below

admin/structure/taxonomy/1/fields

Patch contains some debugging code due to that. Still not sure where exactly it goes wrong.

Still no luck. Recursion still entered for (translated) paths where it shouldn't happen.

YAY! Works for me :)

We still need to resolve the two related core issues tough:
#742318: Fields are editable regardless of whether an bundle instance exists, missing menu titles, etc.
#744258: admin/structure/taxonomy paths have to use machine_name, not vid

And if anyone is interested in this for D6, the following patch just needs to be backported:
#631550: Stale + improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB

Aside from those, let's also look at the remaining @todos in this patch:

+++ admin_menu.inc	16 Mar 2010 19:56:59 -0000
@@ -7,6 +7,363 @@
+function admin_menu_tree($menu_name) {
...
+  foreach ($expand_map as $path => $data) {
+    // Convert named placeholders to anonymous placeholders, since the menu
+    // system stores paths using anonymous placeholders.
+    // @todo Why specify named placeholders in the first place then?
+    $replacements = array_fill_keys(array_keys($data['arguments'][0]), '%');
+    $data['parent'] = strtr($data['parent'], $replacements);
+    $new_map[strtr($path, $replacements)] = $data;
...
+  $expand_map = $new_map;

Unless I overlooked an important detail, the named placeholder arguments are immediately destroyed here, and rebuilt dynamically during the tree merging. If that is the case, I'd say we should use anonymous placeholders in the first place?

+++ admin_menu.inc	16 Mar 2010 19:56:59 -0000
@@ -7,6 +7,363 @@
+  // Unlikely, but possible.
+  if (empty($plids)) {
+    return array();
+  }

This becomes more likely in combination with access permission optimizations (see below).

+++ admin_menu.inc	16 Mar 2010 19:56:59 -0000
@@ -7,6 +7,363 @@
+  // The retrieved menu link trees have to be ordered by depth, so parents
+  // always come before their children for the storage logic below.
+  // @todo Order by 'fit' or 'depth' instead for parent path handling below?
+  foreach ($p_columns as $column) {
+    $query->orderBy($column, 'ASC');
+  }

This @todo is most likely obsolete - we need the links in their true hierarchical order.

+++ admin_menu.inc	16 Mar 2010 19:56:59 -0000
@@ -7,6 +7,363 @@
+      // Set up path arguments map; depends on whether the item is dynamic
+      // (contains placeholders) or not.
+      if (strpos($link['path'], '%') === FALSE) {
+        // Build static item and subtree.
+        $map = explode('/', $link['path']);
+        $item = admin_menu_translate($link, $map);
+        admin_menu_merge_tree($item, $tree_dynamic, $current_expand_map, $hidden);
+        $tree[$key]['below'] += $item;
+      }
+      else {

Since all links we are merging are dynamic items containing placeholders, I can't see to which items this code should apply...?

+++ admin_menu.inc	16 Mar 2010 19:56:59 -0000
@@ -7,6 +7,363 @@
+            $newpath = strtr($path_dynamic, $replacements);
+            // If any placeholder couldn't be replaced, skip this item.
+            if (strpos($newpath, '%') !== FALSE) {
+              continue;
+            }
+            $map = explode('/', $newpath);
+            $item = admin_menu_translate($link, $map);
+            // No access.
+            if (empty($item)) {
+              continue;
+            }
...
+ * @todo We have links. Consider forking menu_link_translate() instead.
+ */
+function admin_menu_translate($router_item, $map) {

Actually, _menu_link_translate() does very similar stuff like we do here. But it seems we need a mixture of both.

+++ admin_menu.inc	16 Mar 2010 19:56:59 -0000
@@ -7,6 +7,363 @@
+            // Build subtree using current replacement arguments.
+            // @todo Avoid rebuilding this for each item.
+            $new_expand_map = array();
+            foreach ($replacements as $placeholder => $value) {
+              $new_expand_map[$placeholder] = array($value);
+            }

Tinkered about this for quite some time, but couldn't see a way to avoid it. Thus, let's just remove the @todo?

+++ admin_menu.inc	16 Mar 2010 19:56:59 -0000
@@ -7,6 +7,363 @@
+    // @todo Strip potential HTML from titles?
+    $router_item['title'] = strip_tags($router_item['title']);

When going through _menu_item_localize(), then the resulting 'link_title' should be sanitized (and translated).

+++ admin_menu.map.inc	16 Mar 2010 19:58:56 -0000
@@ -0,0 +1,141 @@
+ * @todo Replace all/most of those API functions with direct DB queries;
+ *   we only need the menu arguments (keys), not fully loaded objects.
...
+function filter_admin_menu_map() {
+  $map['admin/config/content/formats/%filter_format'] = array(
+    'parent' => 'admin/config/content/formats',
+    'hide' => 'admin/config/content/formats/list',
+    'arguments' => array(
+      array('%filter_format' => array_keys(filter_formats())),
+    ),
+  );
+  return $map;
+}

Most dynamic items are probably located below a parent that is using proper access restrictions - e.g. in this case, admin/config/content/formats is only accessible to users with "administer filters" permission. menu_tree_all_data() should already leave inaccessible items out.

Thus, we could use an early-out return for implementations like this, so we do not even try to merge in those dynamic items.

Actually, filter_formats() will only return those text formats the current user is able to access in D7 (the "administer filters" permission no longer works like a "bypass" permission). So we definitely want to query the available formats ourselves here.

The same probably applies to all or most other implementations - we just need a list of available items. Access is handled elsewhere. And to optimize performance, certain implementations may be skipped entirely by checking a user permission upfront.

Powered by Dreditor.

Since the existing patch already works, I decided to commit the patch as is, so it's easier to track any further changes (in this patch).

Resolves most @todos, but also introduces new ones.

As of now, I assume that the cause for the not contained "Comment fields" and "Comment display" items is the following:

- Node bundle field edit paths are

admin/structure/types/manage/%node_type/fields

- Comment bundle field edit paths are

admin/structure/types/manage/%comment_node_type/comment/fields

Note the additional "comment/" argument in there.

admin/structure/types/manage/%/comment is not a registered path, so it is neither contained in the regular menu tree, but also not in our dynamic.

Thanks, committed!

#17/#18 still needs work.

Thanks! Committed #22 to HEAD.

We still need to resolve dynamic Field UI paths for comments on node types, so back to active.

Re-rolled #16 against HEAD.

Thanks for reporting, reviewing, and testing! Committed #24 to HEAD.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

The missing expansion of "Comment fields" and "Comment display" still remains.

Attached patch is what I have right now. Contains lots of debugging code, but essentially, %comment_node_type does not get expanded, since we seem to have a general problem of only allowing one dynamic argument per path.

Stopping here. The cause for all this trouble seems to be http://api.drupal.org/api/drupal/modules--comment--comment.module/functi..., which pretends to be a loader function, but is a _to_arg function in reality. Thus, no node type loaded or returned... not sure whether we can support such horribly broken code.

Nevertheless it might be a good idea to commit the drupal_array_merge_deep() change.